-
-
Notifications
You must be signed in to change notification settings - Fork 51
Migrate solve
dispatches from DiffEqBase to NonlinearSolveBase
#669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
8dbacdd
to
d6e39bd
Compare
@@ -12,7 +12,7 @@ using LineSearch: BackTracking | |||
using StaticArraysCore: SArray | |||
|
|||
using CommonSolve: CommonSolve | |||
using DiffEqBase: DiffEqBase # Needed for `init` / `solve` dispatches | |||
#using DiffEqBase: DiffEqBase # Needed for `init` / `solve` dispatches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a comment, just delete.
@@ -1,3 +1,449 @@ | |||
const allowedkeywords = (:dense, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The utilities at the top here are reused by DiffEqBase, NonlinearSolveBase, OptimizationBase (in the near future), etc. Let's move these to SciMLBase and share them across the different repos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The allowedkwargs handling, the special error types, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so too, but I figured it might be nice for each to have a separate list of acceptable keywords, so if you tried to pass e.g. dtmin
in to a NonlinearSolve it would give a nice error. Also the messages right now are specific to DiffEqs, they point to the DiffEq docs. I made it so that if there's a mismatch kwarg for a nonlinear solve it points to the NonlinearSolve docs.
But yeah if these are better in SciMLBase I can just put them there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also a lot of small utility functions that live in DiffEqBase right now, e.g.
get_concrete_problem
, extract_alg
, promote_u0
, anyeltypedual
etc. that get used in the solve functions. I'm thinking those should also be moved to SciMLBase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list of acceptable kwargs, yes that one should be problem specific, that would be a nice improvement.
But the others yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the definitions for functions like promote_u0
and anyeltypedual
etc. are in different extensions in DiffEqBase. I'm thinking I should create corresponding extensions in SciMLBase for those packages and move the functions to those. But I'll keep the parts of the extensions specific to DiffEqs in the extensions in DiffEqBase.
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context